-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DBP: Support new dataSource property #2267
Conversation
# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # LocalPackages/DataBrokerProtection/Package.swift # LocalPackages/LoginItems/Package.swift # LocalPackages/NetworkProtectionMac/Package.swift # LocalPackages/PixelKit/Package.swift # LocalPackages/SubscriptionUI/Package.swift # LocalPackages/SwiftUIExtensions/Package.swift # LocalPackages/SyncUI/Package.swift # LocalPackages/SystemExtensionManager/Package.swift # LocalPackages/XPCHelper/Package.swift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bunn I have some questions about some broker JSON files
@@ -45,7 +45,9 @@ | |||
"separator": "," | |||
}, | |||
"profileUrl": { | |||
"selector": ".link-to-details" | |||
"selector": ".link-to-details", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I think this needs to be bumped to 0.1.6.
@brianhall, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole versioning thing was inherited and needs to be defined a little better, I'll work on it. 😄 Technically the identifier isn't visible to any client, so it won't matter if they see it, but happy for this to get bumped to 0.1.6
, I'll add a commit with all changes based on the feedback in a minute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brianhall Let me change the wording. What I meant with “users who find this”, I meant that if a user has this extract JSON broker version on their computers, they will not get this identifier change, because for them it will be the same version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brianhall, I believe it matters because we only parse/save the new JSON if there's a version change. The JSON is not always read from the file; we store it serialized in the DB. Therefore, if we don't bump the version here, we won't send the identifier to C-S-S.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks both! That's useful context, understood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a commit with all changes based on the feedback in a minute.
Just to confirm, are you doing a commit on this branch or in the json repo for me to fetch the changes here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, sorry. Just bumped a commit here and will reflect back in the broker json repo.
{ | ||
"name": "FastPeopleSearch", | ||
"url": "fastpeoplesearch.com", | ||
"version": "0.1.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ This is weird; why is this a new broker, but the version is 0.1.5? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is simply due to the nature of how the generator works; it sets the same version for all of them. I don't see it as an issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's right @Bunn. I think we need to decide whether the versioning is "a versioned release of one or more changes" (the mode I've been operating under) or if each json file gets its own individual version. I've also been operating with the former mental model.
@@ -1,7 +1,7 @@ | |||
{ | |||
"name": "Inforver", | |||
"url": "inforver.com", | |||
"version": "0.1.4", | |||
"version": "0.1.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inforver now redirects to persontrust.com, so I didn't bother updating the identifier, it's ok as-is.
@@ -1,7 +1,7 @@ | |||
{ | |||
"name": "OfficialUSA", | |||
"url": "officialusa.com", | |||
"version": "0.1.4", | |||
"version": "0.1.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bunn Tests are not compiling on this PR
@jotaemepereira I fixed the tests. Other than that, I believe that updating the version of the JSON files to take advancedbackgroundchecks.com changes into consideration is the only thing left right? The discussion about versioning is being held here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍🏼 . Thanks for addressing the comments
# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # LocalPackages/DataBrokerProtection/Package.swift # LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Model/Actions/EmailConfirmation.swift # LocalPackages/NetworkProtectionMac/Package.swift # LocalPackages/SubscriptionUI/Package.swift
Task/Issue URL: https://app.asana.com/0/1204167627774280/1206602738361283/f
Description:
Add new datasource property support
Steps to test this PR:
(Use C-S-S from main to test this)
ex:
Check if we're correctly sending the dataSource property back to CCF
You can do this by breakpointing here
macos-browser/LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/CCF/DataBrokerProtectionFeature.swift
Line 127 in 55375b3
Internal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation